fix: authzen#442
Conversation
Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
📝 WalkthroughWalkthroughThis PR adds two new environment variable bindings for authz configuration (endpoint and cedar policy directory) in the CLI setup, and instruments the AuthZen client's constructor and request methods with structured startup, latency, and decision-outcome logging. ChangesAuthZen observability and config
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AuthZen
participant PDP as AuthZen Endpoint
Caller->>AuthZen: Evaluate(subject, resource, action)
AuthZen->>AuthZen: record start time
AuthZen->>PDP: post(request body)
alt POST fails
PDP-->>AuthZen: error
AuthZen-->>AuthZen: Warnw(error, identifiers)
else POST succeeds
PDP-->>AuthZen: response(status, body)
AuthZen-->>AuthZen: Debugw(status)
AuthZen-->>AuthZen: decisionFrom(response)
AuthZen-->>AuthZen: Debugw(decision, latencyMs)
end
AuthZen-->>Caller: decision
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves the AuthZen authorization driver’s observability and wires up missing environment variable bindings so AuthZ configuration can be provided via env vars.
Changes:
- Add initialization and per-request logging (latency, allow/deny) for the AuthZen PDP driver.
- Add debug logging for outbound AuthZen requests/responses.
- Bind additional AuthZ-related environment variables (
authz_endpoint,authz_cache_ttl,authz_cedar_policy_dir) in the CLI startup.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/authz/authzen.go | Adds AuthZen driver init logging plus request/decision/latency debug logs for evaluate calls. |
| cmd/root.go | Binds additional AuthZ-related environment variables so config is read from the environment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return fmt.Errorf("authz: marshal authzen request: %w", err) | ||
| } | ||
| a.logger.Debugw("authz: authzen request", "endpoint", endpoint, "body", string(buf)) |
| logger.Infow("authz: authzen driver initialized", | ||
| "evalURL", a.evalURL, "evalsURL", a.evalsURL, "wellKnownURL", a.wellKnownURL, | ||
| "timeout", defaultAuthzenTimeout.String()) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/authz/authzen.go (2)
224-229: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFull request body logged unconditionally — potential PII/sensitive-data leak.
post()logs the entire outgoing JSON body (string(buf)) on every call. This body is built from caller-suppliedSubject,Resource, andcontextdata (toEvaluation), which can contain PII or other sensitive attributes. Even at Debug level, this data can end up in log aggregation systems once debug logging is enabled for troubleshooting — a real compliance risk for an authorization service.Consider redacting/omitting sensitive fields (e.g., log only subject/resource type+ID, not full
properties/context) rather than the raw serialized payload.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/authz/authzen.go` around lines 224 - 229, The AuthZen post() method is logging the full serialized request payload via a.Debugw, which can leak sensitive Subject/Resource/context data. Update post() to avoid emitting string(buf) directly and instead log only safe metadata from the request built by toEvaluation, such as endpoint plus redacted subject/resource identifiers or types. If detailed tracing is needed, ensure any logged fields are explicitly sanitized before reaching the logger.
224-229: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDebug log arguments are eagerly evaluated on every request, even when Debug is disabled.
string(buf)performs a full copy of the serialized body on every call topost(), regardless of whether Debug logging is actually enabled — Go evaluates theDebugwarguments before zap's internal level check runs. Sincepost()is invoked on every single authz decision, this adds an unconditional allocation/copy to a hot path.Guard with an explicit level check (e.g.
a.logger.Desugar().Core().Enabled(zapcore.DebugLevel)) before building the body log, or use a zap field type that defers serialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/authz/authzen.go` around lines 224 - 229, The debug body log in AuthZen.post is doing an unconditional string(buf) conversion and allocation on every authz request even when debug logging is off. Update post to avoid building the serialized body unless debug is actually enabled, either by guarding the authz: authzen request log with an explicit zap debug-level check on a.logger or by switching to a deferred zap field approach, so the hot path no longer pays the copy cost when Debugw would be dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/authz/authzen.go`:
- Around line 152-165: The Evaluate logging in authzen.go is emitting the raw
subject identifier via s.ID in both the Warnw and Debugw paths. Review the
authz: authzen evaluate failed and authz: authzen decision logs in the Evaluate
flow and either confirm the backend/retention policy allows this, or replace
s.ID with a hashed/redacted form before logging. Keep the rest of the context
fields intact so the logging remains useful without exposing potentially
PII-bearing subject IDs.
---
Outside diff comments:
In `@internal/authz/authzen.go`:
- Around line 224-229: The AuthZen post() method is logging the full serialized
request payload via a.Debugw, which can leak sensitive Subject/Resource/context
data. Update post() to avoid emitting string(buf) directly and instead log only
safe metadata from the request built by toEvaluation, such as endpoint plus
redacted subject/resource identifiers or types. If detailed tracing is needed,
ensure any logged fields are explicitly sanitized before reaching the logger.
- Around line 224-229: The debug body log in AuthZen.post is doing an
unconditional string(buf) conversion and allocation on every authz request even
when debug logging is off. Update post to avoid building the serialized body
unless debug is actually enabled, either by guarding the authz: authzen request
log with an explicit zap debug-level check on a.logger or by switching to a
deferred zap field approach, so the hot path no longer pays the copy cost when
Debugw would be dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9bbd9124-4433-4b57-be2b-a674c491e91f
📒 Files selected for processing (2)
cmd/root.gointernal/authz/authzen.go
| start := time.Now() | ||
| var resp authzenDecisionResponse | ||
| if err := a.post(ctx, a.evalURL, toEvaluation(s, action, r, reqCtx), &resp); err != nil { | ||
| a.logger.Warnw("authz: authzen evaluate failed", | ||
| "subject", s.ID, "subjectType", s.Type, "action", action, | ||
| "resource", r.Type, "resourceID", r.ID, "error", err) | ||
| return Decision{}, err | ||
| } | ||
| return decisionFrom(resp), nil | ||
| dec := decisionFrom(resp) | ||
| a.logger.Debugw("authz: authzen decision", | ||
| "subject", s.ID, "subjectType", s.Type, "action", action, | ||
| "resource", r.Type, "resourceID", r.ID, "allow", dec.Allow, "reason", dec.Reason, | ||
| "latencyMs", float64(time.Since(start).Microseconds())/1000.0) | ||
| return dec, nil |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Subject ID logged on every Evaluate call — verify it isn't PII.
Unlike Evaluations (which only logs aggregate counts), Evaluate's Warnw/Debugw calls log s.ID directly. Depending on the IdP, subject IDs may be emails or other identifying values. Given the compliance-focused nature of this service, confirm this is acceptable for the target logging backend/retention policy, or hash/redact the identifier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/authz/authzen.go` around lines 152 - 165, The Evaluate logging in
authzen.go is emitting the raw subject identifier via s.ID in both the Warnw and
Debugw paths. Review the authz: authzen evaluate failed and authz: authzen
decision logs in the Evaluate flow and either confirm the backend/retention
policy allows this, or replace s.ID with a hashed/redacted form before logging.
Keep the rest of the context fields intact so the logging remains useful without
exposing potentially PII-bearing subject IDs.
Summary by CodeRabbit
New Features
Bug Fixes